tools: ship every tool on, switchable from one settings map - #710
Conversation
… engine `main` has been red since a69dfd9 (7 commits). Four independent failures were stacked, and only the first was visible: `check-file-size.sh` runs before the Rust toolchain is even installed, so fmt/clippy/doc/test never executed and the three compile/doc breaks behind it went unreported. 1. **file-size ratchet.** 13 grandfathered files grew past their recorded ceilings without the baseline being regenerated. Fixed the way the script itself prescribes — `make file-size-update`. No entry added, none obsolete; still 32 grandfathered files. 2. **`stella-store` did not compile (8 errors).** #704 and #707 independently implemented #616's `store.db` retention. #704 landed `stella-store/src/retention.rs` + `retention_tests.rs` + `stella-cli/src/storage_prune.rs` (`stella storage prune`); #707 landed `stella-store/src/prune.rs` + `stella stats prune`. #707's engine won and swapped `pub mod retention;` for `pub mod prune;` in lib.rs — but left #704's two *consumers* wired up, so `retention.rs` became an orphan outside the module tree while `mod retention_tests;` and `mod storage_prune;` kept importing `crate::retention`. `prune.rs` is unambiguously the survivor: `AGENTS.md` documents `stella stats prune` with exactly its semantics, it has an end-to-end witness in `stella-cli/tests/stats_prune_cli.rs`, and it is cross-referenced from `stella-store/src/lib.rs`, `usage.rs`, and `usage_cmd.rs`. The `retention` path cited no issue and was referenced only by its own files. So this completes the supersession rather than reviving the loser: the three files and their `main.rs` wiring are deleted. 3. **`cargo doc -D warnings` failed in `stella-store`.** prune.rs's module docs intra-doc-linked `dependent_tables_cover_every_execution_keyed_table`, which is `#[cfg(test)]` and so unresolvable when rustdoc builds without test cfg. Named in backticks instead, with a note saying why it is not a link. 4. **`cargo doc -D warnings` failed in `stella-tools`.** A redundant explicit link target in policy.rs (`[`catalog::group_for`](crate::catalog::group_for)` — label and target resolve identically). Collapsed to the plain path form. Two behavioural notes, deliberate in #707 and made permanent by dropping #704's tests, called out because they are easy to miss in a red-main repair: unfinished (in-flight) executions are no longer protected from pruning — prune.rs argues a NULL `finished_at` is exactly the debris retention should reclaim — and a pending enterprise export no longer holds an execution back; the export ledger is instead excluded from the cascade outright, since `executions.id` is AUTOINCREMENT and ids are never reused. Verified locally: check-no-scratch, check-action-pins, check-file-size, `cargo fmt --check`, `cargo clippy --workspace --all-targets -D warnings`, and `RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps` all pass.
Retires the per-capability flags. `Availability::Bash` and `::Web` were policy
defaults wearing an availability costume — neither named a prerequisite, only a
default — which is why every "should this be on?" question needed another enum
variant, another `RegistryOptions` field, and another hand-written branch.
`Availability` now means environment prerequisites only (a media key, a search
key, an issue backend); whether a satisfiable tool is *allowed* is
`ToolPolicy`'s business.
- `bash` and the key-free web family are `Always`. `RegistryOptions` loses
`bash`/`web` entirely. `always_on()` 42 → 46.
- `"tools"` in settings.json is an open map: any tool name, group name, or `*`
→ on/off. Same syntax operators already type; it just stops hardcoding two
keys. Covers MCP (`mcp`) and customer-registered tools (`custom`) too.
- `PolicyToolSet` enforces it directly below `DiscoveryToolSet` — above MCP,
custom, and interactive — so `tool_search` cannot advertise a withheld tool.
It filters `schemas()` AND refuses `execute()`: hiding is a prompt-budget
measure, gating is a capability boundary, and the two must not be confused.
- `candidate_ws.rs` gets it too. Best-of-N candidates build their own registry
from the same options, so post-flip they were a bypass.
- `stella tools` names the key that withheld each tool instead of two
hardcoded "disabled (default)" blocks.
The managed ceiling needed more than union-of-denials, which the witnesses
caught: a managed `{"process": "off"}` is a group key and a project
`{"start_process": "on"}` is more specific, so the union left the org's denial
standing while the tool ran anyway. `apply_tool_ceiling` now also drops any
merged grant the ceiling would deny.
Witnesses verified against the old code in a detached worktree — six fail
there, including `bash_is_registered_with_no_options_at_all` (the flip itself)
and four proving the old `ToolsSettings` silently dropped any key that was not
`bash`/`web`. Stated honestly: `{"bash":"off"}` hiding-and-refusing is a
regression pin, not a fail-on-old witness — bash was absent by default before
too. Its group and custom-tool siblings are the discriminating ones.
Refs #615
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Additional Suggestion:
The process-free branch of run_turn drives the engine on the raw ToolRegistry without wrapping it in PolicyToolSet, so the "tools" policy (operator/managed-org tool switches) is not enforced in the interactive REPL and --no-pipeline one-shot when process-free authority is active.
The settings surface for the tool policy. `views/settings.rs` said "as more
config surfaces move here they become sections of this tab" — this is the
second section.
The deck had no tool list at all, so one now flows from the driver. The catalog
alone would not do: MCP tools and customer-registered custom tools only exist
at runtime, and being able to switch off your own tools is the point. The
driver sources from the live base executor + custom tools + the catalog's
session set, and re-derives the effective policy from disk on every refresh —
`cfg.tool_policy` is frozen at session start, so reading it would have made a
save invisible until restart.
Rows are grouped by catalog group, with `mcp` and `custom` as ordinary sections
so a customer sees their own tools by name. Each row names the key and the
scope that switched it off. Keys are `engine.rs`'s vocabulary verbatim —
`space`/`⏎` toggle, `x` clear, `s`/`S` save user/project, `r` reload, `Esc`
release — with `t` to focus, mirroring `e`.
Org-managed denials render locked and refuse to toggle, and the *writer*
refuses a grant the ceiling denies. Both halves, because a UI that offers a
switch which silently will not work misrepresents the posture.
Two details worth naming:
- `ToolRow` has no `enabled` field. On/off is derived from the switches plus
unsaved edits, so there is one answer rather than two that can drift. The
resolver checks the pending edit at each precedence level before the saved
switch, so a pending *group* grant cannot outrank a saved *exact* denial —
the naive "edits first, then base" version gets that backwards.
- `ToolsSettings::save_to` mirrors `AgentEngineConfig::save_to`: read as a
`Value`, replace only `"tools"`, symlink-reject, 0600 for user scope. An
emptied section removes the key rather than writing `{}`. The panel sends
only changed keys and the driver read-modify-writes, so saving to one scope
cannot copy another scope's switches into it.
Attribution needed more than `disabled_by()`: that yields the key, but the
*scope* is unrecoverable from the merged policy — merging is what destroys it.
Hence `Settings::load_tool_scopes` / `ToolScopePolicies`, which also exposes
`managed_tool_ceiling()` outside the settings module for the first time.
Witnesses are by mutation, since every surface here is new: each named test was
confirmed to flip red when the behaviour it pins is removed (the lock
short-circuit, the exact-vs-group key choice, the key-preserving write, the
writer's ceiling refusal, the tab rendering the panel). That pass caught a weak
witness of its own — `toggling_a_tool_writes_its_exact_name_never_its_group`
originally toggled `bash`, whose group is also `bash`, so it could not tell the
two keys apart; it now toggles `start_process` and asserts `send_stdin` is
untouched.
Rendering was verified by drawing the real deck into a `TestBackend` at 160x30
and 90x30, focused and unfocused, with a pending group edit and a locked row.
That found one defect: a truncated MCP name ran into the state column. The
snapshot test writes its frame to `$CARGO_TARGET_TMPDIR` so it stays
inspectable.
Also moves `mod tool_policy;` to `main.rs`, closing the in-repo FOLLOW-UP the
previous commit left behind.
Refs #615
Merging main into this branch brought in #709's `stella storage prune` without the arm that `run_storage`'s later `match cmd` needs, so the branch did not compile. `run_storage` already returns for `Prune` before the storage map loads — retention operates on `store.db`, not on the map — but exhaustiveness checking is not flow-sensitive, so the arm has to exist even though it is genuinely unreachable. Verified the base commit fails `cargo check -p stella-cli --tests` on its own, so this is the merge's breakage, not the tools editor's.
|
Update: the TUI settings-tab editor landed here too, so this PR now covers the whole decision rather than leaving the UI as a follow-up. (The "Not in scope" note above is stale on that point — per-tool subcommand deny lists remain out of scope, as intended.)
Three things worth a reviewer's attention:
Witnesses here are by mutation (every surface is new): each named test was confirmed to flip red when the behaviour it pins is removed. That pass caught a weak witness of its own — the exact-vs-group test originally toggled Also note the last commit repairs the branch: merging |
…aw `ToolRegistry` without wrapping it in `PolicyToolSet`, so the `"tools"` policy (operator/managed-org tool switches) is not enforced in the interactive REPL and `--no-pipeline` one-shot when process-free authority is active.
This commit fixes the issue reported at stella-cli/src/agent.rs:2058
## Bug
In `stella-cli/src/agent.rs`, `run_turn` branches on `process_free_authority_active()`:
```rust
let outcome = if crate::enterprise_telemetry::process_free_authority_active() {
let engine =
Engine::with_sleeper(provider, registry, engine_config_for(cfg), &TokioSleeper)
.with_calibration(calibration);
engine.run_turn_with_sender(messages, budget, &tx).await
} else {
// ... builds CustomToolSet -> InteractiveToolSet -> PolicyToolSet -> DiscoveryToolSet
let permitted = PolicyToolSet::new(&interactive, session_tool_policy(cfg));
...
};
```
The process-free branch hands the raw `registry` (`&ToolRegistry`) straight to the engine, **never** wrapping it in `PolicyToolSet`. The `else` branch does apply `PolicyToolSet::new(&interactive, session_tool_policy(cfg))`.
### Concrete trigger
`run_turn` is invoked from the interactive REPL loop and from `run_raw_one_shot` (`--no-pipeline`). In process-free mode, `ToolRegistry` still registers many non-host tools (`read_file`, `write_file`, `edit_file`, `apply_edits`, `delete_file`, `save_memory`, task tools, `generate_svg`, etc. — only the host-reaching class is withheld at construction). So with an operator/managed-org policy such as:
```json
{ "tools": { "write_file": "off" } }
```
or
```json
{ "tools": { "*": "off", "read_file": "on" } }
```
the model can **still** call the disabled tools on this path, because no `PolicyToolSet` gates them. Every other driver path (`pipeline` in `agent/goal.rs`, `command_deck.rs`, `subsession.rs`, `fleet_cmd.rs`, `run_pipeline_one_shot`) applies `PolicyToolSet` unconditionally — this was the only path with the gap, breaking the stated invariant that the policy sits above the whole session tool stack on every path.
## Fix
Wrap `registry` in `PolicyToolSet::new(registry, session_tool_policy(cfg))` in the process-free branch before handing it to the engine, mirroring the other paths:
```rust
let permitted = PolicyToolSet::new(registry, session_tool_policy(cfg));
let engine =
Engine::with_sleeper(provider, &permitted, engine_config_for(cfg), &TokioSleeper)
.with_calibration(calibration);
engine.run_turn_with_sender(messages, budget, &tx).await
```
This is type-safe: `&ToolRegistry` already coerces to `&dyn ToolExecutor` (it was passed directly to `Engine::with_sleeper`), which is what `PolicyToolSet::new` accepts, and `&PolicyToolSet` implements `ToolExecutor` (it is wrapped by `DiscoveryToolSet` in the `else` branch). `PolicyToolSet` and `session_tool_policy` are already imported/used in the same function. The process-free layer intentionally strips MCP/custom/interactive/discovery layers, but policy enforcement is preserved.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: macanderson <mac@macanderson.com>
…dropped (#717) `fmt + clippy + test` failed in 7s on `feat/tool-switches-on-709`. As on main, `check-file-size.sh` runs before the Rust toolchain installs, so the fast fail hid a genuine compile error behind it. Three fixes: 1. **`stella-cli/src/settings.rs` crossed the 1500-line ratchet at 1623 lines.** It is not grandfathered, and the guard hard-blocks that case while explicitly forbidding a baseline entry — "the baseline only covers files that predate the guard, and this is the rule that stops the tree getting worse." So it is split, not exempted: the 868-line `#[cfg(test)] mod tests` moves to `stella-cli/src/settings/tests.rs`. That directory already holds seven sibling modules, and the same split has precedent in e72b02f (deck_ui.rs) and in stella-store's `*_tests.rs` files. settings.rs drops to 756 lines, the new file is 874 — neither needs an entry. Pure move, verified as such: the extracted body is byte-identical to the old inline body modulo one dedent level, all 27 `#[test]`s are still present, and `cargo fmt --check` passes on the result untouched. `use super::*` resolves to `crate::settings` from a file submodule exactly as it did inline, so nothing about name resolution changes. 2. **`StorageCmd::Prune(_)` was missing from `run_storage`'s match** — `error[E0004]: non-exhaustive patterns`. The merge commit 6e61207 kept the variant and its early-return dispatch but dropped the `unreachable!` arm. `origin/main` still has that arm (main.rs:1305) and main is green, so this was introduced by this branch's conflict resolution, not inherited. Restored. 3. **Four grandfathered files grew** — agent_tests.rs +157, agent.rs +32, candidate_ws.rs +24, command_deck.rs +1 — so the baseline is regenerated per the script's own instruction. It also *tightened* five ceilings where this branch shrank files (registry.rs 3052→3014, main.rs, stella-store/lib.rs, media.rs, and one more), so the ratchet moves net-inward. Still 32 entries. Note the ordering: regenerating the baseline before fix (2) records a ceiling that fix (2) then exceeds by the two lines it adds. Baseline last. Gate, full workspace: check-no-scratch, check-action-pins, check-file-size, `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps`, and `cargo test --workspace` — 3,559 tests across 61 suites, 0 failures.
…doc link
runs before the Rust toolchain installs, so its fast fail kept hiding the rest.
1. **The regenerated baseline was recorded against the wrong tree.** It has
`command_deck.rs` at 4466 and `deck_ui.rs` at 3940 — main's values — but
this branch's TUI tools editor puts them at 4592 and 3965. Six grandfathered
files were still over ceiling. Regenerated against the actual tree; the
raises this branch really owes are now visible, per the guard's own rule:
command_deck.rs 4465→4592 (+127) the `tools` section: `handle_tools_input`
and `tool_policy_inbound`, beside their
inline sibling `handle_engine_config_input`
deck_ui.rs 3940→3965 (+25) tools panel state on the deck
agent.rs 2223→2224 (+1) the process-free `PolicyToolSet` wrap
main.rs 1776→1782 (+6) the Prune arm's fuller comment
engine.rs 1650→1652 (+2) `t` alongside `e` in the key row
deck_render.rs 1670→1671 (+1)
command_deck.rs's +127 is the one worth a reviewer's eye. It is left inline
deliberately: `handle_tools_input` is the exact structural peer of
`handle_engine_config_input` directly above it, and hoisting only the newer
of the two into a submodule would split a matched pair to satisfy a counter.
2. **`StorageCmd::Prune(_)` was duplicated, not missing** — `warning:
unreachable pattern`, which `-D warnings` makes fatal. #717 read the merge as
having *dropped* main's last-position arm, but the merge had *moved* it to
the top of the match with a fuller comment, so re-adding it made two. Removed
the re-added copy; the branch's own better-documented arm at the head stands.
3. **`views/settings.rs` intra-doc-links private `SPLIT_MIN_WIDTH`** —
`-D rustdoc::private_intra_doc_links` fails the doc build. The prose only
names the threshold, so it drops to plain code formatting rather than widen
the constant's visibility for a comment's sake.
Gate, full workspace: check-no-scratch, check-action-pins, check-file-size,
`cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`,
`RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps`, and
`cargo test --workspace` — 3,579 tests across 61 suites, 0 failures.
…-709 # Conflicts: # CHANGELOG.md
|
CI is green — #717 fixed the first failure but four more were stacked behind it.
One thing for your call
Also flagging that the process-free |
#724) `main` is red at 872ff39. `check-file-size.sh` fails before the Rust toolchain installs, so the whole gate is dark behind it: stella-tui/src/deck_render.rs grew to 1677, over its ceiling of 1676 (+1) stella-tui/src/deck_ui.rs grew to 4049, over its ceiling of 4025 (+24) Neither PR was wrong on its own — this is a collision. #710 and #722 both touch the deck, and #722's baseline was computed against a tree that predated #710's merge. #710 landed first and raised `deck_ui.rs` to 4025; #722 then merged on top and added its own growth over a ceiling that had moved underneath it. Two individually-green PRs, red once combined — the exact shape the merge queue exists to catch (see ci.yml's `merge_group` note), which means neither was queued. Baseline regenerated against the actual tip. Only the two entries move; the other 29 are untouched and the count drops 32→31 (#710's split of settings.rs retired an entry). Worth a second opinion: the guard prefers a new module to a raised ceiling for non-irreducible growth, and `deck_ui.rs` at 4049 is the tree's second-worst file. This raise is the minimum that unblocks a red `main` — +24 and +1, both genuine feature lines from #722 — but if the intent is to hold the line on the deck specifically, the extraction is the better follow-up and this can be reverted into it. Gate, full workspace at this commit: check-no-scratch, check-action-pins, check-file-size, `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps`, and `cargo test --workspace` — 3,605 tests across 61 suites, 0 failures.
…he shell (#764) ## What this is The ask was "fix all P1 issues in one PR." That is not possible as stated, and the reason is structural rather than a matter of effort — so this PR lands the part that *is* landable and puts the rest in front of you as decisions. I classified all **105 open checkbox rows** across the six P1 issues against the code on `main`: | Issue | Open rows | Unblocked | Decision-gated | Already done | |---|---|---|---|---| | #618 public API / protocol break | 29 | 4 | **23** | 2 | | #617 on-disk state | 6 | 2 | 3 | 1 | | #616 budgets & caps | 26 | 2 | **23** | 1 | | #615 trust boundary | 13 | 5 | 7 | 1 | | #612 async boundary | 31 | 4 | **23** | 4 | | #306 CI merge queue | — | repo-settings change, owner-only | | | | **Total** | **105** | **17** | **79** | **9** | **79 of 105 rows cannot be closed by anyone but you**, and the issues say so themselves — each opens with "What decision this asks for." #618's two answers (take one coordinated protocol break vs. declare the types frozen) lead to opposite diffs and it explicitly notes freezing is "equally valid and much cheaper." #616's rows are blocked on *numbers* nobody has written down. #617 needs a ruling on two destructive operations. Guessing any of these wrong is expensive, so I didn't. This PR is the **17 unblocked rows minus the cloud/serve surface you asked me to skip** — and it grew, because closing one of them surfaced three further real bugs on the same path. ## #617 items 8 + 10 — four races on the workspace-open path Item 8 was "the version read is outside the transaction." Fixing it and adding the test the issue asked for by name (four threads, one fresh path) turned up three more, all real: 1. **`apply_migration` re-applied a step another process had already stamped.** Now `IMMEDIATE`, re-reading `user_version` under the write lock. Losing the race is now the no-op it should always have been — which is also what makes a future non-replay-safe step (`ALTER TABLE … ADD COLUMN`) safe to append. 2. **`Store::open` failed outright with `UNIQUE constraint failed: enterprise_export_migration.singleton`** — an existence check followed by an unguarded INSERT. Now `ON CONFLICT DO NOTHING`, which is what its own sibling branch already did. 3. **`PRAGMA journal_mode=WAL` ran before `busy_timeout` was set**, and SQLite skips the busy handler on a WAL upgrade — so the one statement needing an exclusive lock was the one statement with no retry budget. Now behind a bounded retry that mirrors `stella-media`'s `initialize_journal_database`, whose doc comment already described this exact hazard. The main store never got the same treatment. 4. **The generated `.stella/.gitignore` used check-then-rename**, so concurrent openers all renamed over the target; a racer validating the winner's file saw `nlink() == 0` and failed closed with "must be an owner-controlled single-link regular file" — for a file nothing had attacked. Now `O_EXCL | O_NOFOLLOW` exclusive create; losers read the settled file. Two new tests pin that a planted symlink is still rejected and that the file stays committable-mode (0644), not 0600. **Verified end-to-end, not just in tests.** Five concurrent `stella init` runs against one fresh workspace: | | `database is locked` | |---|---| | `origin/main` | **3** | | this branch | **0** | Every fix was negative-controlled by reverting it and confirming the specific failure came back. The four-thread test went 40/40 and the store suite 6/6 under full parallel load. The same version-read-outside-the-transaction shape was sitting as an **unfixed audit note** in `stella-context` and `stella-fleet` — the fleet one explicitly warning that a future non-replay-safe step would be applied twice. Both now use `IMMEDIATE`, and the notes describe what the code does. ## #615 — the shipped docs inverted a security claim #710 moved `bash` and the key-free web family to registered-by-default and never swept the prose. Every release since has told operators the opposite of the surface they actually had: - `README.md` ("**Bash is opt-in.** The default tool surface has no _free-form shell tool_") - `AGENTS.md`, `stella-tools/README.md`, `docs/why-stella.md`, `docs/design/semantic-resolution-bridge.md` - **`docs/design/threat-model.md`** — "**`bash` is opt-in**… the default surface has no shell at all" - **the model's own system prompt** — `tool_steering!` carried "There is no shell unless the workspace enables it", and `prompt.rs`'s own test *pinned that false sentence in place* A false claim in a threat model is worse than no claim, and a false one in the system prompt is read every turn. `web.rs` is the sharpest case: it justified having **no SSRF guard** (localhost, cloud metadata endpoints reachable) on the grounds that the family was opt-in — "the gate is the settings opt-in, not a network allowlist." That gate no longer exists. R4 in the threat model says the same. Both now record it as an **open ruling** rather than presenting a retired guarantee as current. I did not add an SSRF guard: that's strict-vs-permissive and would break "fetch my internal dev server." New `stella-tools/tests/doc_truth.rs` gates all of it, so the claim cannot invert silently again. It caught one instance I'd missed while I was writing it. **Item 1 (the `run_tests` → `bash -c` injection) was already fixed in #624.** I verified it properly rather than taking the checkbox's word: per-token `shell_quote`, and the test runs a real `bash -c` with a negative control. Tick it and don't reopen. **Not fixed, recorded instead:** `screenshot`, `ci_status` and `start_work_on_issue` compose `bash -c` lines that never reach `command_line_for`, so a policy denying command execution cannot see three real shell invocations — contradicting the MUST stated right there in the doc comment. None is injectable (all shell-quote). I left it as a note at that function because closing it makes those tools **newly deniable by an existing operator policy**, which is an operator-visible change that should land deliberately. ## Three small ones - **#618 item 5** — journal failures were reported as `MediaError::Artifact`, pointing operators at `.stella/artifacts/` for a fault in the journal sidecar. New `Journal` variant. (Safe: `MediaError` isn't `Serialize` and has no exhaustive match anywhere.) - **#618 item 17** — the speculation `select!` had `unreachable!` on a provider-driven path. Now a typed terminal error; invariant 5 outranks asserting a structural claim. `ProviderError::Terminal` already existed, so the "fresh error-semantics decision" this row was deferred on was already moot. - **#616 item 8** — `upsert` handed the embedder every missing body in one request, so a full-workspace first index put the whole corpus in a single call. Batched at `warm::BATCH`, the width the warm indexer has always used. Witness test asserts no request exceeds it; negative-controlled at 133 texts. ## Things worth your attention - **#617 item 12 is destructive and I did not touch it.** The orphan-sidecar sweep would `remove_dir_all` session dirs containing `history.json` — the LLM conversation, irrecoverable, and the only thing making a session continuable. The false-positive path is concrete: `list()` swallows unparsable records with `.ok()?`, and `upsert`'s own comment documents that a power cut can leave a zero-length `<id>.json`. **The exact failure the fsync work was added to survive is the one that makes a live session look orphaned and get deleted.** It needs an explicit go/no-go. - **#617 item 5 is destructive too, and the issue doesn't say so.** Retrofitting FKs onto a deployed `fleet.db` can't succeed while orphan rows exist — `apply_migration` hard-fails on `pragma_foreign_key_check` — so the only way through is deleting fleet history. - **#618's break-vs-freeze decision has a cheaper answer than it looks.** `publish = false` is set workspace-wide, so "public API break" always means an in-repo compile-and-fix, never an external consumer — the same premise that already retired three of that issue's four checked rows. - **9 rows are already done** and should just be ticked: #618 items 23, 29 · #617 item 9 · #616 item 29 · #615 item 1, U7 · #612 items 13, 18, 19, 30. - **Duplicate rows:** #616 9/10 are one defect and 13/21 are another. Two rulings close four rows. - **Half the rows cite drifted line numbers.** Anyone landing these should re-locate by symbol. - **#306 (merge queue) is a repo-settings change** — branch protection → "Require merge queue" — that changes how everyone lands PRs, plus a `merge_group` trigger in `ci.yml`. Not something I should flip unilaterally, and the issue itself says to verify the auto-merge interaction against GitHub's current UX rather than assume. ## Gate `make gate` exits 0 (checked directly — piping it to `tail` masks the exit code). Rebased onto `eb663f0d`; `origin/main` had moved 0.5.52 → 0.5.56 under me and #760 restructured `AGENTS.md`, so the doc-truth test and baseline were re-verified after the rebase. The file-size baseline moves **three lines**. `driver.rs` (+11), `stella-store/src/lib.rs` (+13) and `registry.rs` (+5) are in-place code plus the rationale the surrounding style expects, and can't be extracted. The 50-line doc-truth test *was* extracted to its own file rather than raising a baseline for it. --- ## Update — #617 items 12 and 5, per the owner's ruling Both destructive rows are now handled, neither by deleting anything. ### Item 12: fix the conflation, then sweep The sweep was unsafe because `SessionRegistry::list` **cannot tell "no record" from "unreadable record"** — it drops whatever fails to parse, and `upsert`'s own comment records that a power cut leaves a zero-length `<id>.json`. Anything keyed on what `list` accounts for therefore deletes the sidecar of a session whose record was merely truncated. So the conflation is fixed first. `scan()` now reports three states where `list` reported one: | state | meaning | swept? | |---|---|---| | `healthy` | record parsed | no | | `damaged` | `<id>.json` present but unusable (zero-length / truncated) — sidecar intact | **never** | | `orphan_sidecars` | no `<id>.json` beside it at any readability | yes | The orphan test is answered from the **directory entry**, never from a parse. `list()` delegates to `scan().healthy`, so there is one implementation rather than two definitions that can drift. `prune_orphan_sidecars()` re-checks for a record under the same name immediately before removing, so a session that started since the scan is safe. Damaged records are **reported, not repaired**, and the type documents why: `id`, `pid` and `started_at_ms` are recoverable from the filename (ids are self-minted `ses-<ms>-<pid>`), but `workspace` is held nowhere else on disk — not in `journal.jsonl`, not in `history.json` — and resume needs it. Silently hiding the session (what happens today) and silently deleting it (as filed) are both wrong; surfacing it is what's actually available. The witness test asserts the full chain — `list` can't see the record, `scan` calls it damaged with its sidecar noted, the sweep is a no-op, `history.json` survives — and is **negative-controlled against the filed definition** (orphan = "not in `list`"), which classifies that session as an orphan and deletes it. ### Item 5: constrain new ledgers, report on old ones Fresh `fleet.db` files get the four `REFERENCES runs (id)` constraints via a new `FRESH_SCHEMA`, so a ledger created by this build cannot acquire an orphan. Existing files take the unchanged ladder and are never rebuilt — no migration can brick a `fleet.db`, and no history is deleted. The tradeoff is stated at `FRESH_SCHEMA` rather than left implicit: **`SCHEMA_VERSION` no longer determines shape by itself**, since both variants are v2. A future step that rebuilds one of these four tables must reproduce the right variant or read the DDL from `sqlite_master`. That's a real wart; the alternatives were deleting history or leaving new databases unconstrained forever. `Ledger::orphan_rows()` reports what an unconstrained file holds and `enforces_run_references()` says which variant a file is. Rather than land as uncalled API, it's surfaced as a second **`stella doctor`** check. It passes either way — inert rows on an old ledger aren't a fault, and failing the command over them would make `doctor` unusable on exactly the workspaces with history worth keeping. `--repair` deliberately does not act on it, asserted by a test that checks the rows are still present afterwards. Driven against the real binary on a legacy ledger holding 4 orphans across 3 tables: ``` ✓ fleet ledger — .stella/private/fleet.db: 4 row(s) reference a run that is no longer recorded tasks.run_id: 2 row(s) attempts.run_id: 1 row(s) spend.run_id: 1 row(s) → nothing to do — these rows are inert (no query resolves a row by its run) and they are kept because removing them would delete fleet history → this ledger predates the run-reference constraints; a fleet.db created by this build cannot acquire new orphans 2 checks: 2 ok, 0 failed ``` After `stella doctor --repair`: all 4 rows and the $4.20 of recorded spend still there. `doctor`'s module doc claimed "today there is exactly one check" — corrected, and its test now asserts check *names* rather than a count. `make gate` exits 0. Two rustdoc failures caught on the way (a public doc linking a private `FRESH_SCHEMA`, and a `FleetLedger` type that is actually `Ledger`) are fixed.
Stacks on #709 — base it there, or rebase onto main once #709 lands.
mainis currently red on its own (the #704/#707 rival-prune collision); #709 repairs it, and this cherry-picks cleanly on top with the full gate green.Implements the decision: ship with every tool on, and make one switch mechanism work the same way for every tool — built-in, MCP, or customer-registered.
Why the old flags had to go, not just flip
Availability::Bashand::Webwere never availability. Neither named a prerequisite; they named a default. That's why every new "should this be on?" question cost another enum variant, anotherRegistryOptionsboolean, and another hand-written branch in the registry — and why the switch only ever covered built-ins.Split cleanly now:
Availability= what the environment supplies. A media key, a search key, an issue backend.bashand the key-free web tools have no prerequisite, so they areAlways.ToolPolicy= what the operator allows. One open"tools"map insettings.json.The switch
Same syntax operators already type — it just stops hardcoding two keys. One precedence rule: most specific wins, exact name > group >
"*". So{"*": "off", "read_file": "on"}is a read-only agent in two lines.Groups come from
catalog.rs's own section comments, promoted from comments to data. Tools the table has never seen are addressable too:mcp__<server>__<tool>→mcp, anything else →custom, so a customer can switch off their own registered tools without enumerating them.Enforcement
PolicyToolSetsits directly belowDiscoveryToolSet— above MCP, custom, and interactive — the only position that sees the whole session surface. It filtersschemas()and refusesexecute(). That asymmetry is the point: hiding a schema is a prompt-budget measure, but a capability gate has to hold when the name is called anyway, from a stale prompt or a replayed trajectory. (DiscoveryToolSet's lean mode is deliberately the opposite — hides without gating. These must not be confused.) The refusal reads asunknown tool, so a disabled tool is indistinguishable from one that was never built.candidate_ws.rsgets it too — best-of-N candidates build their own registry from the same options, so post-flip they were a bypass.Org control, without new syntax
A deny in the managed scope survives any project grant. The witnesses caught a real hole here:
deny_all_from(union of denials) is not sufficient, because a managed group deny ({"process": "off"}) loses to a more-specific project grant ({"start_process": "on"}).apply_tool_ceilingnow also drops any merged grant the ceiling would deny.Witnesses
Verified in a detached worktree at the base — six fail there:
bash_is_registered_with_no_options_at_allandthe_key_free_web_family_…— the flip itself; both panic with "bash must be advertised with no configuration"ToolsSettingssilently dropped any key that wasn'tbash/webStated plainly:
{"bash":"off"}hiding-and-refusing is a regression pin, not a fail-on-old witness — bash was absent by default before too, so it passes either way. Its group and custom-tool siblings are the discriminating ones.Prose
Every "off by default" claim rewritten:
agent-tools/index.mdx(counts 42→46, new "Switching tools off" section),permissions.mdx,configuration/settings.mdx,configuration/index.mdx,examples/team-settings.mdx, andbash.rs's module docs — which keep the still-true point that switching the tool off removes the shell TOOL, not the shell CAPABILITY (run_tests,run_scriptet al. still reachbash -c; thecommand.startedchain is what covers them uniformly).Not in scope
Per-tool subcommand deny lists (flagging individual verbs of a tool off) — noted as a future direction, deliberately not built here. The TUI settings-tab editor is the remaining piece of the decision;
ToolsSettingsnow derivesSerializespecifically so it can round-trip.Refs #615